Conversation
…a, género y estado del entrenador
4 feat/classes
WalkthroughSe renombra el artefacto a msvc-classes y se eliminan dependencias de springdoc. Se añaden CRUD de clases, nuevos DTOs y mapeadores, repositorio y servicio para clases. Se reestructura Location y Trainer con DTOs request/response, filtros/paginación y reglas de borrado. Se introduce AuthorizationService(+impl) y ampliación de paths públicos (/test/**). Se ajustan paquetes de enums/DTOs y manejo global de excepciones. Configuración multipart añadida. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as ClassController
participant Service as ClassServiceImpl
participant RepoT as TrainerRepository
participant RepoL as LocationRepository
participant Mapper as ClassMapper
participant RepoC as ClassRepository
Client->>Controller: POST /api/classes (ClassRequest)
Controller->>Service: createClass(request)
Service->>RepoT: findById(request.trainerId)
RepoT-->>Service: TrainerEntity | not found
Service->>RepoL: findById(request.locationId)
RepoL-->>Service: LocationEntity | not found
Service->>Mapper: toEntity(request)
Mapper-->>Service: ClassEntity
Service->>Service: set trainer & location
Service->>RepoC: save(entity)
RepoC-->>Service: ClassEntity
Service->>Mapper: toResponse(entity)
Mapper-->>Service: ClassResponse
Service-->>Controller: ClassResponse
Controller-->>Client: 201 Created (ClassResponse)
sequenceDiagram
autonumber
actor Client
participant Controller as LocationController
participant Service as LocationServiceImpl
participant Repo as LocationRepository
participant Mapper as LocationMapper
Client->>Controller: GET /api/locations?page=&size=&search=&active=
Controller->>Service: findAll(page,size,search,active)
alt search && active!=null
Service->>Repo: findByNameContainingIgnoreCaseAndActive(search, active, pageable)
else search
Service->>Repo: findByNameContainingIgnoreCase(search, pageable)
else active==true
Service->>Repo: findByActiveTrue(pageable)
else active==false
Service->>Repo: findByActiveFalse(pageable)
else none
Service->>Repo: findAll(pageable)
end
Repo-->>Service: Page<LocationEntity>
Service->>Mapper: map each to Response
Mapper-->>Service: Page<LocationResponse>
Service-->>Controller: Page<LocationResponse>
Controller-->>Client: 200 OK (Page<LocationResponse>)
sequenceDiagram
autonumber
actor Client
participant Controller as TrainerController
participant Authz as AuthorizationServiceImpl
participant Service as TrainerServiceImpl
participant ClassRepo as ClassRepository
Client->>Controller: DELETE /api/trainers/{id}
Controller->>Authz: canAccessResource(id, authentication)
alt allowed
Controller->>Service: deleteTrainer(id)
Service->>ClassRepo: findFirstByTrainerId(id)
alt has classes
Service-->>Controller: throw IllegalArgumentException
Controller-->>Client: 400/409 Error
else none
Service-->>Controller: void
Controller-->>Client: 200 OK
end
else denied
Controller-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complejo) | ⏱️ ~60 minutos Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/classes/enums/TrainerStatus.java (1)
5-5: Corregir error tipográfico en el enum.El valor del enum
INACTIVotiene una 'o' minúscula en lugar de una 'O' mayúscula. Esto puede causar inconsistencias y errores en el código que espera el valorINACTIVOen mayúsculas completas.Aplique este diff para corregir el error:
- INACTIVo, // No está disponible actualmente + INACTIVO, // No está disponible actualmentesrc/main/java/com/classes/controllers/TrainerController.java (1)
36-64: Corregir expresión @PreAuthorize inválidaEn Line 36 y Line 60 la expresión
@authorizationServiceImpl.canAccessResource(#id, authentication)hace referencia a#id, pero nicreateTrainer(...)nigetAllTrainers()declaran ese parámetro. Spring evalúa la expresión antes de invocar el método y lanzaráSpelEvaluationException, dejando los endpoints inutilizables. Ajusta la anotación para usar únicamente los argumentos disponibles (por ejemplo, una variante que reciba soloauthentication) o introduce el parámetro necesario.
🧹 Nitpick comments (6)
pom.xml (1)
16-17: Alinea el artefacto con el nuevo nombre del microservicioHas renombrado
<name>y<description>amsvc-classes, pero<artifactId>continúa siendomsvc-members. El paquete generado seguirá llamándosemsvc-members-0.0.1-SNAPSHOT.jar, lo que genera divergencia frente al objetivo de renombrar el servicio y puede confundir a los pipelines/deploys. Sugiero actualizar también elartifactId(y, si aplica, ajustar la descripción) para mantener coherencia.- <artifactId>msvc-members</artifactId> + <artifactId>msvc-classes</artifactId> ... - <description>msvc-classes</description> + <description>Microservicio de clases</description>src/main/java/com/classes/services/TrainerService.java (1)
25-28: Eliminar líneas vacías innecesarias.Las líneas vacías al final del archivo son innecesarias y deben eliminarse para mantener la consistencia del código.
- - }src/main/java/com/classes/repositories/TrainerRepository.java (2)
13-15: Eliminar líneas vacías innecesarias.Las líneas vacías al final de la interfaz son innecesarias y deben eliminarse.
public interface TrainerRepository extends JpaRepository<TrainerEntity, UUID> { - - }
7-7: Eliminar import no utilizadojava.util.Optional. Este import no se emplea en la interfaz y puede eliminarse para mantener el código limpio.src/main/java/com/classes/dtos/Class/ClassRequest.java (1)
3-25: Agrega validaciones de Bean Validation.Sin restricciones Bean Validation, una solicitud con datos nulos o valores negativos pasará el
@Validhasta la capa de persistencia y terminará en errores menos controlados (por ejemplo,DataIntegrityViolationException). Anota los campos obligatorios y numéricos con las constraints correspondientes para cortar esos casos en el DTO.import com.fasterxml.jackson.annotation.JsonFormat; +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; import lombok.AllArgsConstructor; import lombok.Data; import lombok.NoArgsConstructor; @@ - private String className; - private UUID locationId; - private UUID trainerId; + @NotBlank + private String className; + @NotNull + private UUID locationId; + @NotNull + private UUID trainerId; @JsonFormat(pattern = "dd-MM-yyyy") - private LocalDate classDate; - private int duration; - private int maxCapacity; - private LocalTime startTime; + @NotNull + private LocalDate classDate; + @Positive + private int duration; + @Positive + private int maxCapacity; + @NotNull + private LocalTime startTime; private boolean active; private String description;src/main/java/com/classes/services/Impl/AuthorizationServiceImpl.java (1)
26-30: Considerar logging para decisiones de autorización.El método
canAccessResourceimplementa lógica de autorización crítica pero no registra las decisiones. Agregar logs puede ayudar en auditorías de seguridad y depuración de problemas de acceso.Considera agregar logging como se muestra:
@Override // Verifica si el usuario es admin o dueño del recurso public boolean canAccessResource(UUID ownerId, Authentication authentication) { - return hasRole(authentication, "ROLE_ADMIN") || ownerId.equals(getUserId(authentication)); + boolean isAdmin = hasRole(authentication, "ROLE_ADMIN"); + boolean isOwner = ownerId.equals(getUserId(authentication)); + boolean hasAccess = isAdmin || isOwner; + + log.debug("Access check for resource {}: isAdmin={}, isOwner={}, granted={}", + ownerId, isAdmin, isOwner, hasAccess); + + return hasAccess; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
pom.xml(1 hunks)src/main/java/com/classes/config/SecurityConfig.java(1 hunks)src/main/java/com/classes/controllers/ClassController.java(1 hunks)src/main/java/com/classes/controllers/HelloController.java(1 hunks)src/main/java/com/classes/controllers/LocationController.java(2 hunks)src/main/java/com/classes/controllers/TrainerController.java(4 hunks)src/main/java/com/classes/dtos/Class/ClassRequest.java(1 hunks)src/main/java/com/classes/dtos/Class/ClassResponse.java(1 hunks)src/main/java/com/classes/dtos/Location/LocationRequest.java(1 hunks)src/main/java/com/classes/dtos/Location/LocationResponse.java(1 hunks)src/main/java/com/classes/dtos/Trainer/FileResponseDTO.java(1 hunks)src/main/java/com/classes/dtos/Trainer/TrainerDTO.java(3 hunks)src/main/java/com/classes/entities/ClassEntity.java(2 hunks)src/main/java/com/classes/entities/TrainerEntity.java(2 hunks)src/main/java/com/classes/enums/ContractType.java(1 hunks)src/main/java/com/classes/enums/DayAvailability.java(1 hunks)src/main/java/com/classes/enums/Gender.java(1 hunks)src/main/java/com/classes/enums/TrainerStatus.java(1 hunks)src/main/java/com/classes/exceptions/GlobalExceptionController.java(2 hunks)src/main/java/com/classes/mappers/ClassMapper.java(1 hunks)src/main/java/com/classes/mappers/LocationMapper.java(2 hunks)src/main/java/com/classes/mappers/TrainerMapper.java(1 hunks)src/main/java/com/classes/repositories/ClassRepository.java(1 hunks)src/main/java/com/classes/repositories/LocationRepository.java(1 hunks)src/main/java/com/classes/repositories/TrainerRepository.java(1 hunks)src/main/java/com/classes/services/AuthorizationService.java(1 hunks)src/main/java/com/classes/services/AzureService.java(1 hunks)src/main/java/com/classes/services/ClassService.java(1 hunks)src/main/java/com/classes/services/CloudinaryService.java(1 hunks)src/main/java/com/classes/services/Impl/AuthorizationServiceImpl.java(1 hunks)src/main/java/com/classes/services/Impl/AzureServiceImpl.java(1 hunks)src/main/java/com/classes/services/Impl/ClassServiceImpl.java(1 hunks)src/main/java/com/classes/services/Impl/CloudinaryServiceImpl.java(1 hunks)src/main/java/com/classes/services/Impl/LocationServiceImpl.java(2 hunks)src/main/java/com/classes/services/Impl/TrainerServiceImpl.java(4 hunks)src/main/java/com/classes/services/LocationService.java(1 hunks)src/main/java/com/classes/services/TrainerService.java(2 hunks)src/main/java/com/classes/services/TrainerServiceImpl.java(0 hunks)src/main/resources/bootstrap.yml(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/classes/services/TrainerServiceImpl.java
🧰 Additional context used
🧬 Code graph analysis (8)
src/main/java/com/classes/dtos/Location/LocationRequest.java (1)
src/main/java/com/classes/dtos/Location/LocationResponse.java (1)
Data(7-14)
src/main/java/com/classes/dtos/Class/ClassResponse.java (1)
src/main/java/com/classes/dtos/Class/ClassRequest.java (1)
Data(12-26)
src/main/java/com/classes/dtos/Location/LocationResponse.java (1)
src/main/java/com/classes/dtos/Location/LocationRequest.java (1)
Data(5-11)
src/main/java/com/classes/controllers/HelloController.java (2)
src/main/java/com/classes/controllers/LocationController.java (1)
RestController(16-65)src/main/java/com/classes/controllers/TrainerController.java (1)
RestController(26-98)
src/main/java/com/classes/controllers/ClassController.java (2)
src/main/java/com/classes/controllers/LocationController.java (1)
RestController(16-65)src/main/java/com/classes/controllers/TrainerController.java (1)
RestController(26-98)
src/main/java/com/classes/dtos/Class/ClassRequest.java (1)
src/main/java/com/classes/dtos/Class/ClassResponse.java (1)
Data(13-28)
src/main/java/com/classes/services/Impl/ClassServiceImpl.java (2)
src/main/java/com/classes/services/Impl/LocationServiceImpl.java (1)
Service(18-81)src/main/java/com/classes/services/Impl/TrainerServiceImpl.java (1)
Service(23-119)
src/main/java/com/classes/controllers/TrainerController.java (1)
src/main/java/com/classes/exceptions/GlobalExceptionController.java (1)
Slf4j(22-137)
🔇 Additional comments (23)
src/main/java/com/classes/services/Impl/CloudinaryServiceImpl.java (1)
3-3: LGTM: Import actualizado correctamente.La actualización del import para
FileResponseDTOes consistente con la reorganización de paquetes. La implementación del servicio permanece sin cambios y funcional.src/main/java/com/classes/services/CloudinaryService.java (1)
3-3: LGTM: Interfaz actualizada correctamente.La actualización del import de
FileResponseDTOmantiene la consistencia con la reorganización de paquetes. El contrato de la interfaz permanece sin cambios.src/main/java/com/classes/mappers/TrainerMapper.java (1)
4-4: LGTM: Import del mapper actualizado correctamente.La actualización del import para
TrainerDTOal paquetecom.classes.dtos.Traineres consistente con la reorganización de DTOs. Los métodos del mapper permanecen sin cambios y funcionales.src/main/java/com/classes/services/Impl/AzureServiceImpl.java (1)
8-8: LGTM: Import actualizado correctamente.La actualización del import para
FileResponseDTOal paquetecom.classes.dtos.Traineres consistente con la reorganización de paquetes. La implementación del servicio de Azure permanece sin cambios y funcional.src/main/java/com/classes/enums/ContractType.java (1)
1-1: LGTM: Convención de nombres de paquetes corregida.El cambio de
com.classes.Enumsacom.classes.enums(minúsculas) sigue las convenciones de nomenclatura de Java y es consistente con la migración de otros enums en el proyecto.src/main/java/com/classes/enums/DayAvailability.java (1)
1-1: LGTM: Convención de nombres de paquetes corregida. No se encontraron referencias acom.classes.Enums.src/main/java/com/classes/dtos/Location/LocationRequest.java (1)
1-11: LGTM: Patrón Request/Response implementado correctamente. Renombramiento deLocationDTOaLocationRequesty reubicación al paquetecom.classes.dtos.Locationson consistentes. No se encontraron referencias al DTO antiguo en el código.src/main/java/com/classes/dtos/Trainer/FileResponseDTO.java (1)
1-1: Reorganización de paquetes consistente
No se encontraron referencias al paquete antiguocom.classes.dtos.FileResponseDTO; el movimiento es seguro.src/main/java/com/classes/enums/Gender.java (1)
1-1: ¡Buen cambio!La corrección del nombre del paquete de
com.classes.Enumsacom.classes.enumssigue las convenciones de nomenclatura de Java (nombres de paquetes en minúsculas).src/main/java/com/classes/enums/TrainerStatus.java (1)
1-1: ¡Buen cambio!La corrección del nombre del paquete sigue las convenciones de nomenclatura de Java.
src/main/java/com/classes/services/AzureService.java (1)
3-3: ¡LGTM!La actualización de la ruta del import alinea con la reorganización de DTOs en el proyecto.
src/main/java/com/classes/config/SecurityConfig.java (1)
33-34: Verificar la necesidad del path raíz "/".La adición del path
/test/**es correcta y alinea con los cambios enHelloController. Sin embargo, el path"/"en la línea 33 puede ser redundante. Verifique si el endpoint raíz debe ser público o si puede eliminarse.src/main/java/com/classes/mappers/LocationMapper.java (2)
4-5: ¡Excelente refactorización!La separación de
LocationDTOenLocationRequestyLocationResponsesigue el patrón de diseño Request/Response, mejorando la separación de responsabilidades y la claridad de la API.
16-23: ¡LGTM!Los métodos de mapeo están correctamente implementados:
toResponse()para convertir entidad a respuestatoEntity()para convertir request a entidadupdateFromRequest()con la estrategia apropiada de ignorar valores nulosLa nomenclatura es clara y consistente.
src/main/java/com/classes/controllers/HelloController.java (1)
9-13: ¡Mejora adecuada!Los cambios implementados son positivos:
- El uso de
ResponseEntity<String>es más apropiado que retornar unStringdirecto, permitiendo un mejor control de las respuestas HTTP.- El cambio de path a
/test/saludoestá correctamente alineado con los cambios enSecurityConfig.javaque permiten acceso público a/test/**.- La adición de
@GetMappinghace el código más explícito y legible.src/main/java/com/classes/services/AuthorizationService.java (1)
7-15: LGTM: Diseño de interfaz bien estructurado.La interfaz
AuthorizationServicedefine claramente los contratos de autorización necesarios con métodos bien nombrados que cubren escenarios comunes (verificación de roles, extracción de ID de usuario, y control de acceso basado en propiedad de recursos).src/main/java/com/classes/dtos/Trainer/TrainerDTO.java (2)
3-6: LGTM: Imports actualizados correctamente.El cambio de
com.classes.Enums.*acom.classes.enums.*sigue las convenciones de nomenclatura de Java donde los paquetes deben estar en minúsculas.
21-21: Verificar compatibilidad de clientes con nuevo formato de fechaNo se encontraron otros usos de
@JsonFormat(pattern="yyyy-MM-dd")en los DTOs, pero cambiar add-MM-yyyypuede romper clientes que esperan ISO. Revisa pruebas de integración y coordina con consumidores del API para confirmar que el nuevo formato no afecta la compatibilidad.src/main/java/com/classes/dtos/Class/ClassResponse.java (1)
13-28: LGTM: DTO bien estructurado.
ClassResponseestá correctamente definido con:
- Anotaciones Lombok apropiadas para reducir boilerplate
- Formato de fecha consistente (
"dd-MM-yyyy") con otros DTOs del PR- Campos descriptivos que incluyen nombres legibles (locationName, trainerName) en lugar de IDs anidados
src/main/java/com/classes/controllers/ClassController.java (1)
40-45: LGTM: Autorización correcta en endpoints con ID.Los métodos
updateClassydeleteClassutilizan correctamente@PreAuthorizecon la variable#idque existe como@PathVariable.Also applies to: 47-52
src/main/java/com/classes/mappers/ClassMapper.java (1)
18-32: LGTM: Mapper bien diseñado con MapStruct.El
ClassMapperestá correctamente implementado con:
- Mapeos explícitos que ignoran campos que se setean en el servicio (location, trainer)
- Mapeo de nombres legibles desde relaciones (location.name → locationName)
- Estrategia
IGNOREpara valores nulos en actualizaciones, evitando sobrescribir campos no enviados- Comentarios útiles que documentan el propósito de cada método
src/main/java/com/classes/repositories/LocationRepository.java (1)
16-24: LGTM: Métodos de consulta con paginación bien implementados.Los métodos agregados siguen las convenciones de Spring Data JPA y proporcionan capacidades útiles de filtrado:
- Búsqueda case-insensitive por nombre
- Filtros por estado activo/inactivo
- Combinación de búsqueda y filtro de estado
- Soporte completo de paginación con
Pageablesrc/main/java/com/classes/services/LocationService.java (1)
11-21: LGTM: Interfaz actualizada con patrón Request/Response.La interfaz
LocationServicese actualizó correctamente para usar DTOs separados:
LocationRequestpara operaciones de entrada (create/update)LocationResponsepara operaciones de salida- Soporte de paginación con
Page<LocationResponse>- Parámetros de filtrado bien definidos (search, active)
Este patrón mejora la separación de responsabilidades y permite evolucionar las APIs de entrada/salida de forma independiente.
| @PreAuthorize("@authorizationServiceImpl.canAccessResource(#id,authentication)") | ||
| @PostMapping | ||
| public ResponseEntity<LocationDTO> create(@RequestBody LocationDTO dto) { | ||
| LocationDTO created = locationService.create(dto); | ||
| public ResponseEntity<LocationResponse> create(@RequestBody LocationRequest request) { | ||
| LocationResponse created = locationService.create(request); | ||
| return new ResponseEntity<>(created, HttpStatus.CREATED); | ||
| } | ||
|
|
||
| @PreAuthorize("@authorizationServiceImpl.canAccessResource(#id,authentication)") | ||
| @GetMapping | ||
| public ResponseEntity<Page<LocationDTO>> findAll( | ||
| public ResponseEntity<Page<LocationResponse>> findAll( | ||
| @RequestParam(defaultValue = "0") int page, | ||
| @RequestParam(defaultValue = "10") int size | ||
| @RequestParam(defaultValue = "10") int size, | ||
| @RequestParam(required = false) String search, | ||
| @RequestParam(required = false) Boolean active | ||
| ) { | ||
| Page<LocationDTO> locations = locationService.findAll(page, size); | ||
| Page<LocationResponse> locations = locationService.findAll(page, size, search, active); | ||
| return ResponseEntity.ok(locations); | ||
| } | ||
|
|
||
| @PreAuthorize("@authorizationServiceImpl.canAccessResource(#id,authentication)") |
There was a problem hiding this comment.
Corregir expresión @PreAuthorize inválida
En Line 24 y Line 31 la expresión @authorizationServiceImpl.canAccessResource(#id,authentication) usa #id, pero create(...) y findAll(...) no exponen ese parámetro. Spring evaluará el SpEL y lanzará SpelEvaluationException, bloqueando cualquier petición a estos endpoints. Ajusta la condición para basarte solo en los argumentos realmente presentes o proporciona el parámetro requerido.
🤖 Prompt for AI Agents
En src/main/java/com/classes/controllers/LocationController.java lines 24-42:
las anotaciones @PreAuthorize referencian #id pero los métodos create(...) y
findAll(...) no tienen ese parámetro, lo que causará SpelEvaluationException;
remueve la referencia a #id y adapta la expresión para usar solo argumentos
presentes (por ejemplo pasar solo authentication:
@PreAuthorize("@authorizationServiceImpl.canAccessResource(authentication)") o
cambiar a una comprobación equivalente que use roles/privilegios disponibles), o
alternativamente expone el parámetro requerido en la firma del método si
realmente debe evaluarse por id.
| @ExceptionHandler(EntityNotFoundException.class) | ||
| public ResponseEntity<ErrorResponse> handleEntityNotFoundException(EntityNotFoundException ex) { | ||
| ErrorResponse errorResponse = new ErrorResponse( | ||
| "ENTITY_NOT_FOUND", | ||
| "No se encontró la entidad solicitada", | ||
| List.of(ex.getMessage()) | ||
| ); | ||
| log.warn("Entity not found: {}", ex.getMessage()); | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(errorResponse); |
There was a problem hiding this comment.
Evita NullPointerException al componer el detalle.
List.of(ex.getMessage()) falla con NullPointerException cuando la excepción no trae mensaje (caso habitual al lanzar new EntityNotFoundException()). El handler terminaría devolviendo 500 y perderíamos el control del error.
- ErrorResponse errorResponse = new ErrorResponse(
- "ENTITY_NOT_FOUND",
- "No se encontró la entidad solicitada",
- List.of(ex.getMessage())
- );
+ String detail = ex.getMessage() != null ? ex.getMessage() : "Entidad no encontrada";
+ ErrorResponse errorResponse = new ErrorResponse(
+ "ENTITY_NOT_FOUND",
+ "No se encontró la entidad solicitada",
+ Collections.singletonList(detail)
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @ExceptionHandler(EntityNotFoundException.class) | |
| public ResponseEntity<ErrorResponse> handleEntityNotFoundException(EntityNotFoundException ex) { | |
| ErrorResponse errorResponse = new ErrorResponse( | |
| "ENTITY_NOT_FOUND", | |
| "No se encontró la entidad solicitada", | |
| List.of(ex.getMessage()) | |
| ); | |
| log.warn("Entity not found: {}", ex.getMessage()); | |
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(errorResponse); | |
| @ExceptionHandler(EntityNotFoundException.class) | |
| public ResponseEntity<ErrorResponse> handleEntityNotFoundException(EntityNotFoundException ex) { | |
| String detail = ex.getMessage() != null ? ex.getMessage() : "Entidad no encontrada"; | |
| ErrorResponse errorResponse = new ErrorResponse( | |
| "ENTITY_NOT_FOUND", | |
| "No se encontró la entidad solicitada", | |
| Collections.singletonList(detail) | |
| ); | |
| log.warn("Entity not found: {}", ex.getMessage()); | |
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(errorResponse); |
🤖 Prompt for AI Agents
In src/main/java/com/classes/exceptions/GlobalExceptionController.java around
lines 126 to 134, the call List.of(ex.getMessage()) can throw a
NullPointerException when ex.getMessage() is null; change the construction of
the details list to handle null safely (e.g., use ex.getMessage() != null ?
List.of(ex.getMessage()) : List.of()) so the handler always returns a valid
ErrorResponse and a 404, and keep the existing log call.
| @Override | ||
| public UUID getUserId(Authentication authentication) { | ||
| String userIdStr = ((JwtAuthenticationToken) authentication).getToken().getClaim("user_id"); | ||
| return UUID.fromString(userIdStr); | ||
| } |
There was a problem hiding this comment.
Agregar validaciones de null y manejo de excepciones.
El método getUserId tiene varios problemas de seguridad en tiempo de ejecución:
- Cast sin verificación de tipo (línea 22): Si
authenticationno es una instancia deJwtAuthenticationToken, se lanzaráClassCastException - Falta validación de null: Si el claim
"user_id"es null,UUID.fromStringlanzaráNullPointerException - Formato inválido: Si el claim no es un UUID válido, se lanzará
IllegalArgumentException
Aplica este diff para agregar las validaciones necesarias:
@Override
public UUID getUserId(Authentication authentication) {
+ if (!(authentication instanceof JwtAuthenticationToken)) {
+ throw new IllegalArgumentException("Authentication must be a JwtAuthenticationToken");
+ }
String userIdStr = ((JwtAuthenticationToken) authentication).getToken().getClaim("user_id");
+ if (userIdStr == null) {
+ throw new IllegalStateException("user_id claim not found in JWT token");
+ }
+ try {
- return UUID.fromString(userIdStr);
+ return UUID.fromString(userIdStr);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid UUID format for user_id: " + userIdStr, e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public UUID getUserId(Authentication authentication) { | |
| String userIdStr = ((JwtAuthenticationToken) authentication).getToken().getClaim("user_id"); | |
| return UUID.fromString(userIdStr); | |
| } | |
| @Override | |
| public UUID getUserId(Authentication authentication) { | |
| if (!(authentication instanceof JwtAuthenticationToken)) { | |
| throw new IllegalArgumentException("Authentication must be a JwtAuthenticationToken"); | |
| } | |
| String userIdStr = ((JwtAuthenticationToken) authentication).getToken().getClaim("user_id"); | |
| if (userIdStr == null) { | |
| throw new IllegalStateException("user_id claim not found in JWT token"); | |
| } | |
| try { | |
| return UUID.fromString(userIdStr); | |
| } catch (IllegalArgumentException e) { | |
| throw new IllegalArgumentException("Invalid UUID format for user_id: " + userIdStr, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/classes/services/Impl/AuthorizationServiceImpl.java around
lines 20 to 24, the getUserId method currently casts without checking and
assumes the "user_id" claim is non-null and a valid UUID; change it to first
verify authentication is an instance of JwtAuthenticationToken and throw an
appropriate exception if not, retrieve the "user_id" claim safely as a String
and validate it's not null or blank, then parse UUID inside a try-catch to catch
IllegalArgumentException and rethrow a clear, informative exception (or return
an Optional/nullable value per project conventions) so ClassCastException,
NullPointerException and invalid-format exceptions are prevented and errors
include context for debugging.
| LocationEntity entity = repository.findById(id) | ||
| .orElseThrow(() -> new RuntimeException("Location not found")); | ||
| mapper.updateFromRequest(request, entity); | ||
| LocationEntity updated = repository.save(entity); | ||
| return mapper.toResponse(updated); |
There was a problem hiding this comment.
Usa una excepción específica para el 404
RuntimeException("Location not found") terminará en un 500 si no existe un handler dedicado, lo que rompe el contrato REST al no devolver 404. Propón usar una excepción de dominio (p.ej. ResourceNotFoundException) o ResponseStatusException(HttpStatus.NOT_FOUND); así el controlador responderá con el código correcto y un mensaje consistente.
🤖 Prompt for AI Agents
In src/main/java/com/classes/services/Impl/LocationServiceImpl.java around lines
37 to 41, the code throws a generic RuntimeException when a Location isn't
found—causing a 500 instead of a 404; replace the RuntimeException with a
domain-specific exception (e.g., throw new ResourceNotFoundException("Location
not found", id)) or throw new ResponseStatusException(HttpStatus.NOT_FOUND,
"Location not found") via the orElseThrow so the REST layer returns 404, and if
you choose ResourceNotFoundException ensure that exception class exists (or
create it) and is mapped by a @ControllerAdvice or appropriate handler so
responses have the correct status and message.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/classes/dtos/Class/ClassResponse.java (1)
10-10: Elimina la importación no utilizada.La importación de
LocalTimeno se utiliza en esta clase, ya que el camposchedulees de tipoString.Aplica este diff para eliminar la importación:
-import java.time.LocalTime;src/main/java/com/classes/mappers/ClassMapper.java (1)
25-25: Formatea explícitamente los valores de tiempo en el schedule.La concatenación directa de objetos
LocalTimemediante el operador+en la expresión depende del métodotoString()deLocalTime, que devuelve el formato ISO-8601 (por ejemplo, "09:30:00"). Aunque funcional, esto puede ser inconsistente con otros formatos de fecha/hora utilizados en la aplicación (como el patrón "dd-MM-yyyy" enclassDate).Considera formatear explícitamente los tiempos para mayor claridad y control:
- @Mapping(target = "schedule", expression = "java(entity.getStartTime() + \" - \" + entity.getEndTime())") + @Mapping(target = "schedule", expression = "java(entity.getStartTime().format(java.time.format.DateTimeFormatter.ofPattern(\"HH:mm\")) + \" - \" + entity.getEndTime().format(java.time.format.DateTimeFormatter.ofPattern(\"HH:mm\")))")O, alternativamente, crea un método auxiliar para mejorar la legibilidad:
default String formatSchedule(LocalTime startTime, LocalTime endTime) { DateTimeFormatter formatter = DateTimeFormatter.ofPattern("HH:mm"); return startTime.format(formatter) + " - " + endTime.format(formatter); }Y actualiza el mapping:
- @Mapping(target = "schedule", expression = "java(entity.getStartTime() + \" - \" + entity.getEndTime())") + @Mapping(target = "schedule", expression = "java(formatSchedule(entity.getStartTime(), entity.getEndTime()))")src/main/java/com/classes/entities/ClassEntity.java (2)
34-40: Especifica la estrategia de carga para evitar problemas de rendimiento N+1.Las relaciones
@ManyToOneparalocationytrainerno especifican una estrategia de carga (fetch), por lo que se utiliza el valor predeterminadoFetchType.EAGER. Esto puede causar consultas N+1 y problemas de rendimiento cuando se cargan múltiples instancias deClassEntity.Considera especificar explícitamente
FetchType.LAZYpara mejorar el rendimiento:- @ManyToOne + @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "location_id") private LocationEntity location; - @ManyToOne + @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "trainer_id") private TrainerEntity trainer;Si necesitas cargar estas relaciones en casos específicos, puedes usar
@EntityGrapho consultas conJOIN FETCHen el repositorio.
26-31: Considera añadir validaciones JPA en los campos de la entidad.Los campos como
duration,maxCapacity,className, etc., no tienen anotaciones de validación (por ejemplo,@NotNull,@Positive,@Size). Aunque las validaciones pueden estar presentes en los DTOs, añadirlas también en la entidad proporciona una capa adicional de protección contra datos inválidos a nivel de persistencia.Ejemplo de validaciones sugeridas:
+ @NotBlank private String className; + @Positive private int duration; + @Positive private int maxCapacity; + @NotNull private LocalTime startTime; + @NotNull private LocalTime endTime;Asegúrate de importar
jakarta.validation.constraints.*si decides implementar estas validaciones.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/classes/dtos/Class/ClassRequest.java(1 hunks)src/main/java/com/classes/dtos/Class/ClassResponse.java(1 hunks)src/main/java/com/classes/entities/ClassEntity.java(2 hunks)src/main/java/com/classes/mappers/ClassMapper.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/classes/dtos/Class/ClassRequest.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/classes/dtos/Class/ClassResponse.java (1)
src/main/java/com/classes/dtos/Class/ClassRequest.java (1)
Data(12-27)
| private String name; | ||
|
|
||
| private String className; |
There was a problem hiding this comment.
🧩 Analysis chain
Aclara el propósito de los campos name y className.
La entidad tiene tanto un campo name (línea 23) como un campo className (línea 25), lo que puede resultar confuso. El resumen indica que el campo title fue renombrado a name, pero también se añadió un nuevo campo className. Esto podría indicar:
- Una refactorización incompleta donde uno de los campos debe ser eliminado, o
- Un diseño intencional donde ambos campos tienen propósitos diferentes (por ejemplo,
namepara uso interno yclassNamepara visualización).
Verifica si ambos campos son necesarios y, de ser así, considera añadir documentación JavaDoc para aclarar la diferencia entre ellos. Si uno es redundante, elimínalo.
Ejecuta el siguiente script para verificar el uso de ambos campos en el código:
🏁 Script executed:
#!/bin/bash
# Buscar usos del campo "name" en ClassEntity
echo "=== Usos del campo 'name' en ClassEntity ==="
rg -nP '\bname\b' --type=java -C3 -g '!**/test/**'
echo ""
echo "=== Usos del campo 'className' en ClassEntity ==="
rg -nP '\bclassName\b' --type=java -C3 -g '!**/test/**'Length of output: 13394
Eliminar campo redundante name en ClassEntity o clarificar su propósito
El campo name en ClassEntity no se usa en ClassRequest, ClassResponse ni en repositorios; elimina name o añade JavaDoc que diferencie claramente su función de className.
🤖 Prompt for AI Agents
In src/main/java/com/classes/entities/ClassEntity.java around lines 23-25, the
private String name field is redundant with className; either remove the name
field (delete its declaration, getters/setters/usages, update constructors,
equals/hashCode, DTO mappings, and any JPA/DB mappings) or keep it but add clear
JavaDoc above the field explaining how it differs from className and rename it
to a more descriptive identifier if needed; update ClassRequest/ClassResponse,
repository queries, and tests accordingly so there is no ambiguity or unused
field left behind.
2 participants
Summary by CodeRabbit
Nuevas funcionalidades
Mejoras
Seguridad
Cambios